feat(auth): add subject to grant #3440

cozminu · 2025-05-27T14:04:38Z

Changes proposed in this pull request

Context

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

netlify · 2025-05-27T14:04:42Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`8e0b9c0`
🔍 Latest deploy log	https://app.netlify.com/projects/brilliant-pasca-3e80ec/deploys/68efae64e149b30008fa0c41

github-actions · 2025-06-02T11:57:08Z

🚀 Performance Test Results

Test Configuration:

VUs: 4
Duration: 1m0s

Test Metrics:

Requests/s: 42.50
Iterations/s: 14.18
Failed Requests: 0.00% (0 of 2556)

📜 Logs


> [email protected] run-tests:testenv /home/runner/work/rafiki/rafiki/test/performance
> ./scripts/run-tests.sh -e test "-k" "-q" "--vus" "4" "--duration" "1m"

Cloud Nine GraphQL API is up: http://localhost:3101/graphql
Cloud Nine Wallet Address is up: http://localhost:3100/
Happy Life Bank Address is up: http://localhost:4100/
cloud-nine-wallet-test-backend already set
cloud-nine-wallet-test-auth already set
happy-life-bank-test-backend already set
happy-life-bank-test-auth already set
     data_received..................: 923 kB 15 kB/s
     data_sent......................: 2.0 MB 33 kB/s
     http_req_blocked...............: avg=7.44µs   min=2.36µs   med=5.55µs   max=942.82µs p(90)=6.77µs   p(95)=7.42µs  
     http_req_connecting............: avg=802ns    min=0s       med=0s       max=902.55µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=93.45ms  min=7.37ms   med=77.04ms  max=571.08ms p(90)=160.15ms p(95)=185.01ms
       { expected_response:true }...: avg=93.45ms  min=7.37ms   med=77.04ms  max=571.08ms p(90)=160.15ms p(95)=185.01ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 2556
     http_req_receiving.............: avg=96.17µs  min=23.16µs  med=82.81µs  max=4ms      p(90)=121.63µs p(95)=156.46µs
     http_req_sending...............: avg=37.26µs  min=10.43µs  med=28.62µs  max=2.44ms   p(90)=41.63µs  p(95)=57.49µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=93.32ms  min=7.22ms   med=76.89ms  max=571.01ms p(90)=160.05ms p(95)=184.86ms
     http_reqs......................: 2556   42.504649/s
     iteration_duration.............: avg=281.73ms min=173.38ms med=267.29ms max=1.09s    p(90)=350.12ms p(95)=381.1ms 
     iterations.....................: 853    14.184846/s
     vus............................: 4      min=4       max=4 
     vus_max........................: 4      min=4       max=4

packages/auth/src/subject/service.ts

mkurapov · 2025-06-12T16:41:05Z

packages/auth/src/grant/utils.test.ts

+jest.mock('../access/types', () => ({
+  isIncomingPaymentAccessRequest: (access: AccessRequest) =>
+    access.type === 'incoming-payment',
+  isQuoteAccessRequest: (access: AccessRequest) => access.type === 'quote'
+}))


Why do we need this mock?

canSkipInteraction function uses them and the purpose of the unit test is to test only the unit and not the deps, but I see there are also benefits from using the unmocked functions. removed the mock

njlie

Overall LGTM, just a couple things

packages/auth/src/graphql/resolvers/grant.ts

njlie · 2025-06-16T17:52:44Z

packages/auth/src/subject/service.test.ts

+  })
+
+  describe('getByGrant', (): void => {
+    test('gets access', async () => {


nit: this is getting subject, not access

mkurapov

Few comments!

mkurapov · 2025-06-23T15:18:35Z

packages/auth/src/grant/routes.ts

+        400,
+        GNAPErrorCode.InvalidRequest,
+        err.message || 'invalid request'


We should match GrantErrors to their respective HTTP codes and GNAPErrorCodes.
This is similar to how @oana-lolea maps AccessErrors to the respective code & error codes in her PR.

mkurapov · 2025-06-23T15:23:33Z

packages/auth/src/grant/utils.ts

+  const emptyAccess = (body.access_token?.access?.length ?? 0) === 0
+
+  if (emptySubject && emptyAccess) {
+    throw new Error('subject or access_token required')


Should this throw a GrantError instead?

yes, adapted to throw GrantError

packages/auth/src/graphql/schema.graphql

mkurapov · 2025-06-23T15:27:07Z

packages/auth/src/accessToken/routes.ts

 interface IntrospectBody {
  access_token: string
  access?: AccessItem[]
+  subjects?: SubjectItem[]


If we don't use subjects anywhere (given that we don't pass it in during the introspection request), then its OK to leave out

packages/auth/src/subject/service.ts

Co-authored-by: Max Kurapov <[email protected]>

njlie

Just one suggestion to be applied accross all the places where trx was cast in order to silence a Typescript error.

njlie · 2025-07-01T08:52:50Z

packages/auth/src/access/service.test.ts

  let appContainer: TestContainer
  let accessService: AccessService
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


It might be healthier to actually initialize trx in the beforeAll block so that the tables get properly truncated when the test is finished, but the editor also doesn't flag a Typescript error. I assume this type casting was done to address the TS error.

beforeAll(async (): Promise<void> => { ... trx = await appContainer.knex.transaction() })

Yes, It was a fix to a TS error

Variable 'trx' is used before being assigned.ts(2454)

Using a transaction is tricky because it can deadlock tests because some queries are done within a transaction and others are not. Managed to fix it, but in another manner.

let trx: KnexOrTransaction beforeAll(async (): Promise<void> => { ... trx = await appContainer.knex })

Is this ok with you?

njlie · 2025-07-01T08:56:04Z

packages/auth/src/access/utils.test.ts

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T08:56:10Z

packages/auth/src/accessToken/routes.test.ts

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T08:56:42Z

packages/auth/src/accessToken/service.test.ts

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:02:17Z

packages/auth/src/grant/service.test.ts

  let appContainer: TestContainer
  let grantService: GrantService
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:03:52Z

packages/auth/src/grant/utils.test.ts

@@ -0,0 +1,86 @@
+import { AccessRequest } from '../access/types'


Thanks for adding these tests! Especially since they cover logic that existed before this PR

njlie · 2025-07-01T09:11:17Z

packages/auth/src/signature/middleware.test.ts

    let interaction: Interaction
    let token: AccessToken
-    let trx: Knex.Transaction
+    const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:21:18Z

packages/backend/src/open_payments/payment/outgoing/service.test.ts

  let client: string
  let amtDelivered: bigint
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

mkurapov · 2025-07-21T16:01:53Z

packages/auth/src/graphql/resolvers/grant.ts

  client: grant.client,
-  access: grant.access?.map((item) => accessToGraphql(item)),
+  access: grant.access?.map((item) => accessToGraphql(item)) || [],
+  subject: grant.subjects?.map((item) => subjectToGraphql(item)) || [],


grant.subjects only exists if we add it to the withGraphFetched request in the service methods OR we have subjects as a separate "child" resolver under grants, and call the subjectService.getByGrantId directly.

An example of this is the Asset > sendingFee resolver in backend.

fixed with withGraphFetched('subjects') in the service methods

mkurapov · 2025-07-21T16:06:09Z

packages/auth/src/graphql/schema.graphql

  "Wallet address of the grantee's account."
  client: String!
  "Details of the access provided by the grant."
-  access: [Access!]!


I think we can keep it as [Access!]!, if there is no access it can simply be an empty array.

mkurapov · 2025-07-21T16:07:56Z

packages/auth/src/grant/routes.ts

      400,
      GNAPErrorCode.InvalidRequest,
-      'access identifier required'
+      err instanceof Error ? err.message : ''


to avoid empty string, let's provide some generic message instead at least. It's less important for the client, its more so for us to find where to debug it.

changed to 'unknown error while checking interaction requirement'

mkurapov · 2025-07-21T16:08:53Z

packages/auth/src/subject/service.ts

+      'subject id must be a valid https url'
+    )
+  }
+  if (subject.format != 'uri') {


Suggested change

if (subject.format != 'uri') {

if (subject.format !== 'uri') {

mkurapov · 2025-07-21T16:23:10Z

packages/auth/src/subject/service.ts

+
+function validateSubjectRequest(subject: SubjectRequest): void {
+  try {
+    if (!subject.id.startsWith('https://')) throw 1


not sure if @njlie agrees, but I think just checking that it is a valid URL is sufficient, without checking the https://. IMO it can be up to the ASE to determine how strict they want to be with the specifics, like protocol.

For the record, I also agree.

mkurapov · 2025-07-21T16:25:40Z

packages/auth/src/grant/model.ts

+  subjectItems: Subject[]
 ): OpenPaymentsGrant {
  return {
    access_token: toOpenPaymentsAccessToken(accessToken, accessItems, {


access_token could be undefined now, just like subject

mkurapov · 2025-07-21T16:28:07Z

packages/auth/src/access/service.test.ts

  let appContainer: TestContainer
  let accessService: AccessService
-  let trx: Knex.Transaction
+  let trx: TransactionOrKnex


We should remove the import { Knex } from 'knex' import

mkurapov · 2025-07-21T16:36:19Z

packages/auth/src/interaction/routes.ts

  ctx.body = {
    grantId: interaction.grant.id,
    access: access.map(toOpenPaymentsAccess),
+    subjects: subjects.map(toOpenPaymentsSubject),


there can only be one subject for a grant, but the sub_ids can be many. So this can either be a subject object with sub_ids array, (like the Open Payments spec), or we can directly return sub_ids here.

The same applies for the subject updates in the GraphQL schema

changed to subject object with sub_ids array.
I'm not aware of a subject updates.

mkurapov · 2025-09-15T17:38:59Z

packages/auth/src/openapi/specs/id-provider.yaml

                properties:
                  access:
                    $ref: ./auth-server.yaml#/components/schemas/access
+                  subject:


We should make this spec standalone, and move over the correct components (access/subject) into this file, similar to how it was done for token-introspection

mkurapov · 2025-10-06T12:20:37Z

packages/auth/src/grant/routes.ts

    const accessToken = await accessTokenService.create(grant.id)
    const access = await accessService.getByGrant(grant.id)


I think we should only create an accessToken if the access_token was provided in the initial grant request. We can use length of access to verify that, I believe. Otherwise, if the client is just requesting subject information, and not access_token, we would return an access_token still, eg:

{ "access_token": { "access": [], "value": "EC6884A68394DAC1E186", "manage": "http://localhost:3006/token/965841c8-1671-4174-944d-56778083dfdb", "expires_in": 600 }, "continue": { "access_token": { "value": "B785F0F5ED51E08A923E" }, "uri": "http://localhost:3006/continue/b094d601-cd62-43fd-9389-5ed6628c1c1d" }, "subject": { "sub_ids": [ { "id": "https://cloud-nine-wallet-backend/accounts/gfranklin", "format": "uri" } ] } }

CC @njlie to confirm as well

Yeah agreed, there's nothing to access, hence no access token.

fixed, access_token is no more when the grant doesn't contain access items

* feat(localenv): expose subject during consent in mock-ase * feat: include client name in subject grant line * fix(mase): grantId not being retrieved * fix(mase): consent and confirmation texts * fix: handle subject-only grants properly --------- Co-authored-by: Cozmin Ungureanu <[email protected]>

mkurapov

The auth server changes look good 👍
I found a few things in the mock ase, but I will put up a change separately

feat(auth): add subject to grant

6b05778

cozminu self-assigned this May 27, 2025

github-actions bot added type: tests Testing related type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels May 27, 2025

feat(auth): add subject to grant

c6a310e

fix(auth): grant access when is undefined

ed8411d

cozminu requested review from mkurapov and njlie June 2, 2025 12:30

mkurapov reviewed Jun 12, 2025

View reviewed changes

njlie reviewed Jun 16, 2025

View reviewed changes

cozminu added 2 commits June 20, 2025 18:12

feat(auth): subject id validation

fdfbba3

fix(auth): fix tests

03b1ec5

github-actions bot added the pkg: backend Changes in the backend package. label Jun 20, 2025

mkurapov reviewed Jun 23, 2025

View reviewed changes

cozminu and others added 3 commits June 25, 2025 11:04

Update packages/auth/src/graphql/schema.graphql

9b224b2

Co-authored-by: Max Kurapov <[email protected]>

feat(auth): throw GrantError instead of Error

d9d7982

fix(auth): description for subIdFormat

7a7d8e6

cozminu requested a review from mkurapov June 25, 2025 14:08

njlie reviewed Jul 1, 2025

View reviewed changes

fix(auth): trx in tests is knex

efd3042

mkurapov reviewed Jul 21, 2025

View reviewed changes

cozminu added 5 commits July 23, 2025 17:14

Merge branch 'main' into cozmin/raf-996

454195e

fix: grant service to throw only grant errors

99e11b9

fix(auth): address change requests

a2db4d6

fix(auth): address change requests

223e69e

fix tests

eb28179

github-actions bot removed the pkg: backend Changes in the backend package. label Jul 28, 2025

cozminu added 2 commits September 5, 2025 16:11

Merge branch 'main' into cozmin/raf-996

805795b

change op specs version

4b6f81c

cozminu dismissed mkurapov’s stale review via 4b6f81c September 9, 2025 14:18

mkurapov reviewed Sep 15, 2025

View reviewed changes

cozminu added 3 commits September 30, 2025 14:34

feat(auth): update idp openapi spec

4f7babc

feat(auth): make idp standalone

f5e9ddf

chore(deps): update axios

763a2e9

github-actions bot added pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-ase labels Oct 1, 2025

cozminu requested a review from mkurapov October 1, 2025 12:38

mkurapov reviewed Oct 6, 2025

View reviewed changes

cozminu added 5 commits October 13, 2025 15:08

fix(auth): access token in response when it shouldnt

2360eea

Merge branch 'main' into cozmin/raf-996

0c2a4ca

fix(auth): tests

8a47595

try to fix grype scan

b4321af

try fix grype

798392e

github-actions bot added the type: ci Changes to the CI label Oct 14, 2025

cozminu and others added 6 commits October 14, 2025 15:26

restore 'fix'

b2927c7

try fix grype

de08468

try fix grype 2

751dad2

Merge branch 'main' into cozmin/raf-996

5ad9be6

Merge branch 'main' into cozmin/raf-996

8e0b9c0

github-actions bot removed pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. labels Oct 15, 2025

cozminu requested a review from mkurapov October 16, 2025 07:21

mkurapov approved these changes Oct 17, 2025

View reviewed changes

cozminu merged commit 539ced6 into main Oct 20, 2025
44 checks passed

cozminu deleted the cozmin/raf-996 branch October 20, 2025 10:48

		@@ -0,0 +1,86 @@
		import { AccessRequest } from '../access/types'

	if (subject.format != 'uri') {
	if (subject.format !== 'uri') {

		const accessToken = await accessTokenService.create(grant.id)
		const access = await accessService.getByGrant(grant.id)

feat(auth): add subject to grant #3440

feat(auth): add subject to grant #3440

Uh oh!

Conversation

cozminu commented May 27, 2025

Changes proposed in this pull request

Context

Checklist

Uh oh!

netlify bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Uh oh!

github-actions bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Performance Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njlie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njlie Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

netlify bot commented May 27, 2025 •

edited

Loading

github-actions bot commented Jun 2, 2025 •

edited

Loading

njlie Jun 16, 2025 •

edited

Loading

mkurapov left a comment •

edited

Loading